test_runner: error on test file patterns that do not exist#64345
test_runner: error on test file patterns that do not exist#64345UditDewan wants to merge 1 commit into
Conversation
Report each literal positional argument that does not match an existing file instead of silently dropping it. Fixes: nodejs#64109
|
Review requested:
|
nodejs-github-bot
added
needs-ci
| if (hasUserSuppliedPattern) { | ||
| if (results.length === 0 && ArrayPrototypeEvery(glob.matchers, (m) => !m.hasMagic())) { | ||
| console.error(`Could not find '${ArrayPrototypeJoin(patterns, ', ')}'`); | ||
| process.exit(kGenericUserError); | ||
| } | ||
|
|
||
| const missing = ArrayPrototypeFilter(patterns, (pattern, i) => { | ||
| return !glob.matchers[i].hasMagic() && !existsSync(resolve(cwd, pattern)); | ||
| }); | ||
|
|
||
| if (missing.length > 0) { | ||
| console.error(`Could not find '${ArrayPrototypeJoin(missing, ', ')}'`); | ||
| process.exit(kGenericUserError); | ||
| } |
There was a problem hiding this comment.
| if (hasUserSuppliedPattern) { | |
| if (results.length === 0 && ArrayPrototypeEvery(glob.matchers, (m) => !m.hasMagic())) { | |
| console.error(`Could not find '${ArrayPrototypeJoin(patterns, ', ')}'`); | |
| process.exit(kGenericUserError); | |
| } | |
| const missing = ArrayPrototypeFilter(patterns, (pattern, i) => { | |
| return !glob.matchers[i].hasMagic() && !existsSync(resolve(cwd, pattern)); | |
| }); | |
| if (missing.length > 0) { | |
| console.error(`Could not find '${ArrayPrototypeJoin(missing, ', ')}'`); | |
| process.exit(kGenericUserError); | |
| } | |
| if (hasUserSuppliedPattern && results.length === 0) { | |
| console.error(`Could not find '${ArrayPrototypeJoin(patterns, ', ')}'`); | |
| process.exit(kGenericUserError); | |
| } |
Wouldn't this suffice?
There was a problem hiding this comment.
Unfortunately not — that check only fires when all patterns collectively match nothing, which is the pre-existing behavior (minus the hasMagic() guard). In the repro from #64109, spec/*.test.js does match files, so results.length > 0 and no-tests-here is still silently dropped. The per-pattern check is what catches a nonexistent literal path even when other patterns match.
I also deliberately kept the hasMagic() guard on both checks so a glob that matches zero files keeps its current no-error behavior — removing it would newly fail runs that pass optional globs. Happy to make globs error too if that's preferred, but it seemed like a separate (and riskier) behavior change.
|
Not sure this is the direction proposed in nodejs/test-runner#13 but this would probably be a
semver-major
|
4 participants
Currently
createTestFileList()only errors when all user-supplied patterns collectively match nothing and none of them contain glob magic. As a result, a nonexistent literal path passed alongside a matching glob is silently dropped:This change adds a per-pattern check: any literal (non-glob) pattern that does not exist on disk is now reported with the existing
Could not find '...'error and the run exits with a failure code. Glob patterns that match nothing are intentionally left as-is so existing workflows with optional globs keep working, and the existing all-patterns check is preserved.Added test cases to
test/parallel/test-runner-cli.jscovering both repros from the issue, for both--test-isolation=noneandprocess.Fixes: #64109